[PR]: Update documentation on temporal averaging, usage of bounds, and generation of weights #601

tomvothecoder · 2024-02-06T17:02:30Z

Description

Closes [Doc]: Add more info about how temporal averaging is performed and how weights are generated #600

~~Weights are determined from the difference in the bounds~~ ) -- already done in docstrings

Lines 170 to 189 in fbf1db6

    
                   Time bounds are used for inferring the time series frequency and for 
        
                   generating weights (refer to the ``weighted`` parameter documentation 
        
                   below). 
        
                   Parameters 
        
                   ---------- 
        
                   data_var: str 
        
                       The key of the data variable for calculating averages 
        
                   weighted : bool, optional 
        
                       Calculate averages using weights, by default True. 
        
                       Weights are calculated by first determining the length of time for 
        
                       each coordinate point using the difference of its upper and lower 
        
                       bounds. The time lengths are grouped, then each time length is 
        
                       divided by the total sum of the time lengths to get the weight of 
        
                       each coordinate point. 
        
                       The weight of masked (missing) data is excluded when averages are 
        
                       taken. This is the same as giving them a weight of 0.

Data is grouped into the labelled time point for averaging operations
If one time point spans across the time intervals that you are averaging into, then weights are not properly assigned
get_time_bounds generally assumes data with frequencies of annual, monthly, daily, or sub-daily (I thought of this while writing this issue)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

If applicable:

I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass with my changes (locally and CI/CD build)
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

tomvothecoder · 2024-02-07T19:21:40Z

Hey @pochedls, this small PR is ready for review whenever you have time.

tomvothecoder

Added "Warning:" to some comments.

xcdat/temporal.py

codecov · 2024-02-07T19:24:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (94271f1) 100.00% compared to head (99327ff) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #601   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1602      1602           
=========================================
  Hits          1602      1602

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pochedls

@tomvothecoder – thanks for taking this on. I've made some suggestions for you to consider.

xcdat/bounds.py

pochedls · 2024-02-15T18:42:49Z

xcdat/bounds.py

+        This method general assumes data with time frequencies of annual,
+        monthly, daily, or sub-daily. It loops over the time axis coordinate
+        variables and attempts to add bounds for each of them if they don't
+        exist.


This is not always true, since the specified method can be "midpoint." Suggest adding text similar to the text in add_missing_bounds where the freq optional argument is documented in the docstring.

While I'm here...could a space be added between the freq and daily_subfreq arguments in the documentation (for consistency)?

I'll copy the text from add_missing_bounds() here.

Typically numpy style docstrings have no spaces in between parameters. I think I added spaces after parameters that use bullet points in the description because it wasn't rendering lists correctly. I removed spaces and the docs still seem to render fine.

I'll go through docstrings and remove spaces if they used for parameters.

@tomvothecoder – I don't have strong feelings about this. I just noticed it wasn't consistent (but now I understand why).

pochedls · 2024-02-15T18:56:48Z

xcdat/temporal.py

+            Warning: If one time point spans across the time intervals that 
+            you are averaging into, then weights are not properly assigned.


I'm debating if this should go here or a Note below. Suggest the following text.

Warning: When using weighted averages, the weights are assigned based on the timepoint value. For example, a time point of 2020-06-15 with bounds (2020-06-01, 2020-06-30) has 30 days of weight assigned to June, 2020 (e.g., for an annual average calculation). This would be expected behavior, but it's possible that data could span across typical temporal boundaries. For example, a time point of 2020-06-01 with bounds (2020-05-16, 2020-06-15) would have 30 days of weight, but this weight would be assigned to June, 2020, which would be incorrect (15 days of weight should be assigned to May and 15 days of weight should be assigned to June). This issue could plausibly arise when using pentad data.

Also make sure new lines are used consistently between docstring parameter entries (here and elsewhere).

Removed this change and added your suggestion to the Notes section.

pochedls · 2024-02-15T19:03:49Z

xcdat/temporal.py

+
+            Warning: If one time point spans across the time intervals that 
+            you are averaging into, then weights are not properly assigned.


Here and elsewhere – should this be a longer entry at the bottom of the docstring (like the example above). Maybe we could also include a brief note, like:

"Note that weights are assigned by the labelled time point. If the dataset includes timepoints that span across typical boundaries (e.g., a timepoint on 2020-06-01 with bounds that begin in May 2020 and end in June 2020), the weights will not be assigned properly. See explanation below."

tomvothecoder

@pochedls I think I addressed your suggestions. Let me know if I'm missing anything.

xcdat/temporal.py

- Add more specific information on how temporal averaging grouping is performed - Add information on assumed time frequencies for `get_time_bounds()` - Add info about how weights are not properly assigned if one time point spans across the time interval being averaged into

Co-authored-by: Stephen Po-Chedley <pochedley@gmail.com>

github-actions bot added the Type: Documentation label Feb 6, 2024

tomvothecoder self-assigned this Feb 6, 2024

tomvothecoder added this to the FY24Q2 (01/01/24 - 03/31/24) milestone Feb 6, 2024

tomvothecoder mentioned this pull request Feb 6, 2024

[Feature]: More Sophisticated Bounds Handling in Temporal Averaging Operations #594

Open

tomvothecoder requested a review from pochedls February 7, 2024 19:21

tomvothecoder commented Feb 7, 2024

View reviewed changes

xcdat/temporal.py Outdated Show resolved Hide resolved

xcdat/temporal.py Outdated Show resolved Hide resolved

xcdat/temporal.py Outdated Show resolved Hide resolved

xcdat/temporal.py Outdated Show resolved Hide resolved

tomvothecoder added type: docs Updates to documentation and removed Type: Documentation labels Feb 12, 2024

tomvothecoder changed the title ~~Update documentation on temporal averaging, usage of bounds, and generation of weights~~ [Pull Request] Update documentation on temporal averaging, usage of bounds, and generation of weights Feb 12, 2024

tomvothecoder changed the title ~~[Pull Request] Update documentation on temporal averaging, usage of bounds, and generation of weights~~ [PR] Update documentation on temporal averaging, usage of bounds, and generation of weights Feb 12, 2024

tomvothecoder changed the title ~~[PR] Update documentation on temporal averaging, usage of bounds, and generation of weights~~ [PR]: Update documentation on temporal averaging, usage of bounds, and generation of weights Feb 12, 2024

pochedls reviewed Feb 15, 2024

View reviewed changes

github-actions bot added the Type: Documentation label Feb 16, 2024

tomvothecoder commented Feb 16, 2024

View reviewed changes

xcdat/temporal.py Outdated Show resolved Hide resolved

tomvothecoder commented Feb 16, 2024

View reviewed changes

xcdat/temporal.py Outdated Show resolved Hide resolved

pochedls approved these changes Feb 19, 2024

View reviewed changes

tomvothecoder and others added 6 commits February 21, 2024 09:44

Apply suggestions from code review

aee1c2a

Apply suggestions from code review

5af9ae5

Co-authored-by: Stephen Po-Chedley <pochedley@gmail.com>

Add Steve"s suggestions

c3d1c29

Remove unnecessary space after docstring bullet points

fd36dae

Apply suggestions from code review

99327ff

tomvothecoder force-pushed the doc/600-temp-avg-info branch from c17b8fd to 99327ff Compare February 21, 2024 17:45

tomvothecoder merged commit 78dedf0 into main Feb 21, 2024
9 checks passed

tomvothecoder deleted the doc/600-temp-avg-info branch February 21, 2024 17:51

tomvothecoder restored the doc/600-temp-avg-info branch February 21, 2024 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PR]: Update documentation on temporal averaging, usage of bounds, and generation of weights #601

[PR]: Update documentation on temporal averaging, usage of bounds, and generation of weights #601

tomvothecoder commented Feb 6, 2024 •

edited

Loading

tomvothecoder commented Feb 7, 2024

tomvothecoder left a comment

codecov bot commented Feb 7, 2024 •

edited

Loading

pochedls left a comment

pochedls Feb 15, 2024

pochedls Feb 15, 2024

tomvothecoder Feb 16, 2024 •

edited

Loading

pochedls Feb 16, 2024

pochedls Feb 15, 2024

pochedls Feb 15, 2024

tomvothecoder Feb 16, 2024

pochedls Feb 15, 2024

tomvothecoder left a comment

	Time bounds are used for inferring the time series frequency and for
	generating weights (refer to the ``weighted`` parameter documentation
	below).

	Parameters
	----------
	data_var: str
	The key of the data variable for calculating averages

	weighted : bool, optional
	Calculate averages using weights, by default True.

	Weights are calculated by first determining the length of time for
	each coordinate point using the difference of its upper and lower
	bounds. The time lengths are grouped, then each time length is
	divided by the total sum of the time lengths to get the weight of
	each coordinate point.

	The weight of masked (missing) data is excluded when averages are
	taken. This is the same as giving them a weight of 0.

		Warning: If one time point spans across the time intervals that
		you are averaging into, then weights are not properly assigned.

[PR]: Update documentation on temporal averaging, usage of bounds, and generation of weights #601

[PR]: Update documentation on temporal averaging, usage of bounds, and generation of weights #601

Conversation

tomvothecoder commented Feb 6, 2024 • edited Loading

Description

Checklist

tomvothecoder commented Feb 7, 2024

tomvothecoder left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

pochedls left a comment

Choose a reason for hiding this comment

pochedls Feb 15, 2024

Choose a reason for hiding this comment

pochedls Feb 15, 2024

Choose a reason for hiding this comment

tomvothecoder Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

pochedls Feb 16, 2024

Choose a reason for hiding this comment

pochedls Feb 15, 2024

Choose a reason for hiding this comment

pochedls Feb 15, 2024

Choose a reason for hiding this comment

tomvothecoder Feb 16, 2024

Choose a reason for hiding this comment

pochedls Feb 15, 2024

Choose a reason for hiding this comment

tomvothecoder left a comment

Choose a reason for hiding this comment

tomvothecoder commented Feb 6, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

tomvothecoder Feb 16, 2024 •

edited

Loading